Add PKCS#11 3.2 bindings + plug them into the cryptoki #264

Jakuje · 2025-04-25T13:58:23Z

The PKCS#11 3.2 headers are available and considered final:

https://github.com/latchset/pkcs11-headers/tree/main/public-domain/3.2

This pulls the new headers, regenerates bindings and adjusts the initialization so they can be used.

I took also the chance to update bindgen to 0.71.1 once we generate new binding version

Jakuje · 2025-04-25T14:05:04Z

Oh, lovely! The CI does not like the generated binding as it is too complex.

error: very complex type used. Consider factoring parts into `type` definitions
    --> /home/runner/work/rust-cryptoki/rust-cryptoki/cryptoki-sys/src/bindings/x86_64-unknown-linux-gnu.rs:6801:35
     |
6801 |       pub C_UnwrapKeyAuthenticated: Result<
     |  ___________________________________^
6802 | |         unsafe extern "C" fn(
6803 | |             arg1: CK_SESSION_HANDLE,
6804 | |             arg2: *mut CK_MECHANISM,
...    |
6814 | |         ::libloading::Error,
6815 | |     >,
     | |_____^
     |

Who would say it for a function with 10 arguments:

    pub C_UnwrapKeyAuthenticated: Result<
        unsafe extern "C" fn(
            arg1: CK_SESSION_HANDLE,
            arg2: *mut CK_MECHANISM,
            arg3: CK_OBJECT_HANDLE,
            arg4: *mut CK_BYTE,
            arg5: CK_ULONG,
            arg6: *mut CK_ATTRIBUTE,
            arg7: CK_ULONG,
            arg8: *mut CK_BYTE,
            arg9: CK_ULONG,
            arg10: *mut CK_OBJECT_HANDLE,
        ) -> CK_RV,
        ::libloading::Error,
    >,

Is this something we should fix in the generated bindings or open an issue for the bindgen? Not sure if this is a new issue with the new bindgen, but more likely the issue of the new PKCS#11 API having that insane number of arguments ....

wiktor-k · 2025-04-29T19:53:36Z

Hmm fortunately this is just a clippy issue. I guess suppressing it somewhere would be an okayish workaround. (maybe in lib.rs of -sys?)

I'd still report it to bindgen. This may improve their code generation or at least make them consider adding the lint suppression in the generated code.

Jakuje · 2025-04-30T07:33:25Z

Thanks for the pointer! I see the lib.rs has already some more warnings ignored. I am not sure if there is some way how to improve the code generation to avoid this. I can think of using some temporary named type for the function, but I think it just moves the problem one more function argument away.

I think this is inherent issue of the PKCS#11 specification, introducing this complex functions.

wiktor-k · 2025-04-30T09:08:33Z

I am not sure if there is some way how to improve the code generation to avoid this.

FWIW I didn't mean that. Slapping the lint suppression there is sufficient enough for me.

Jakuje · 2025-05-05T17:02:53Z

Kryoptic tests now fail as there are few bugs in the last release that were already fixed in main. I think this is already good for first reviews, but I would probably wait for the pkcs11 3.2 to get released officially and I will likely implement also the SLH-DSA (they look quite simple and similar to the ML-DSA, but I do not have an implementation to test against yet).

hug-dev · 2025-05-12T08:59:25Z

Feel free to ping us when you want us to review this again!

Jakuje · 2025-06-19T07:26:52Z

I think this should be ready for reviews! Rebased and updated links to the final headers.

teythoon · 2025-06-25T14:48:36Z

I found a small issue. When I ask for an AttributeType::MlDsaParameterSet, the token returns an Attribute::ParameterSet.

I think the patch introduces AttributeType::MlDsaParameterSet as an alias for AttributeType::ParameterSet. Both MlDsaParameterSet and ParameterSet map to CKA_PARAMETER_SET, and therefore the distinction is lost, and the response will be a ParameterSet.

Jakuje · 2025-06-25T14:52:53Z

I found a small issue. When I ask for an AttributeType::MlDsaParameterSet, the token returns an Attribute::ParameterSet.

I think the patch introduces AttributeType::MlDsaParameterSet as an alias for AttributeType::ParameterSet. Both MlDsaParameterSet and ParameterSet map to CKA_PARAMETER_SET, and therefore the distinction is lost, and the response will be a ParameterSet.

Yeah, I was aware of this issue, but I am not sure how this could be easily handled given that this attribute has different values for different key types. Any thoughts?

I think it would require some logic in the GetAttributeValue mapping the type to the correct aliased "subtype" based on the key type, but I really did not look into that yet.

teythoon · 2025-06-26T11:07:23Z

I think the aliasing is most surprising. The Rust model is a refinement of the interface model, i.e. it is more expressive. I think that is a mistake at this point. It is also surprising for people coming from the PKCS#11 spec.

Instead, I think we should offer explicit conversion functions from ParameterSetType (doesn't exist yet, and should hold a u64, and that is what Attribute::ParameterSet should use) to MlDsaParameterSet.

wiktor-k

Looks good 👌 Thanks for attaching tests for various combinations of the new API.

cryptoki/src/context/general_purpose.rs

cryptoki/src/mechanism/mldsa.rs

wiktor-k · 2025-07-07T10:43:58Z

cryptoki/src/mechanism/mldsa.rs

+        let (p_context, ul_context_len) = match context {
+            Some(c) => (
+                c.as_ptr() as *mut _,
+                c.len().try_into().expect("usize can not fit in CK_ULONG"),
+            ),
+            None => (null_mut(), 0),
+        };


Suggested change

let (p_context, ul_context_len) = match context {

Some(c) => (

c.as_ptr() as *mut _,

c.len().try_into().expect("usize can not fit in CK_ULONG"),

),

None => (null_mut(), 0),

};

let (p_context, ul_context_len) = if let Some(c) = context {

(c.as_ptr() as *mut _,

c.len().try_into().expect("usize can not fit in CK_ULONG")),

} else {

(null_mut(), 0),

};

};

I tend to avoid matches which then destructure into Some/None as I think if let is more readable... but it's not a blocker :)

wiktor-k · 2025-07-07T10:45:42Z

cryptoki/tests/common/mod.rs

@@ -16,6 +16,7 @@ fn get_pkcs11_path() -> String {
        .unwrap_or_else(|_| "/usr/local/lib/softhsm/libsofthsm2.so".to_string())
 }

+#[allow(dead_code)]


Is that still needed?

warning: function `is_softhsm` is never used --> cryptoki/tests/common/mod.rs:19:8 | 19 | pub fn is_softhsm() -> bool { | ^^^^^^^^^^ | = note: `#[warn(dead_code)]` on by default warning: `cryptoki` (test "ml_kem") generated 1 warning warning: `cryptoki` (test "ml_dsa") generated 1 warning (1 duplicate)

I did some restructure of the tests and this reports failure for the two now (mlkem and mldsa) as they do not use this public function.

I think the whole test structure can be improved as now it builds separate test binaries, which is not very effective.

So as a after-thought I think we should really have one test binary and source code split to separate files, probably something similar as we have in kryoptic (but that lives under src/tests, which I understand is not a standard place to push tests according to rust.

which is not very effective.

Oh yeah, this is definitely an annoying aspect of cargo test and part of the reason why in other projects we're instead using cargo nextest run instead of bare cargo test.

hug-dev

Thank you for pushing for this!

cryptoki/src/context/mod.rs

Jakuje · 2025-07-08T12:25:38Z

I still want to resolve the ParameterSetType issue reported by @teythoon, but I needed some clarification what exactly he had in mind. I agree that the current solution is wrong, but I was not able to make better one working so far :)